-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collapsed qualifiers kg #268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @beasleyjonm, this looks great. I think we should go ahead and implement this in a way that would handle all qualifiers though. That way we don't need to worry about keeping this up to date when we inevitably have more, and it would include the couple others we already have.
I added a few comments here for how we could do that pretty easily.
Common/collapse_qualifiers.py
Outdated
# TODO - really we should get the full list of qualifiers from Common/biolink_constants.py, | ||
# but because we currently cannot deduce the association types of edges and/or permissible value enumerators, | ||
# we have to hard code qualifier handling anyway, we might as well check against a smaller list | ||
QUALIFIER_KEYS = [OBJECT_DIRECTION_QUALIFIER, OBJECT_ASPECT_QUALIFIER] | ||
# we do have these qualifiers but we cant do any redundancy with them so ignore for now: | ||
# QUALIFIED_PREDICATE - | ||
# SPECIES_CONTEXT_QUALIFIER - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments aren't as applicable to collapse_qualifiers as they were for the redundant graph. For the redundant graph we needed to be able to look up the ancestors of the value of the qualifier, but to do that you need to know the qualifier value enum_name, and I wasn't sure how to derive that given an edge from ORION, so we hard coded what we needed at the time (which is still not ideal). In this case, we only care about qualifiers and values which are actually on the edge, so we should be able to capture them all. We can do this dynamically easily using the biolink model toolkit.
Getting the full list of qualifiers from the constants file was a bad idea anyway, they shouldn't be hard coded if the biolink model toolkit can supply the current list. I'm not sure why I said that. :)
Common/collapse_qualifiers.py
Outdated
|
||
# qualifiers = check_qualifier(edge) <- it would be better to do something like this but because we're not | ||
# handling other qualifiers anyway it's faster to just do the following: | ||
qualifiers = [qualifier for qualifier in QUALIFIER_KEYS if qualifier in edge] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using this hard coded list, we can do something like the following (except you would want to instantiate the toolkit outside of the loop):
from Common.biolink_utils import get_biolink_model_toolkit
bmt = get_biolink_model_toolkit()
qualifiers = {key:value for key, value in edge.items() if bmt.is_qualifier(key)}
Then you could do something like the following, with a function that does your semantic transformations where applicable
for qualifier, qualifier_value in qualifiers.items():
qualifier_statement += semantic_adjustment(qualifier, qualifier_value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. I've added a new commit that handles all of qualifiers. There is still some hard-coded decisions involved with the current qualifiers, but it will print a warning if new qualifiers that aren't handled here are ever found in edges.jsonl files in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping through the qualifiers once you determine them like I mentioned would be better because it's more efficient (it would only attempt to handle the qualifiers that are actually there instead of checking if every possible qualifier is on every edge), and cleaner/more maintainable, mostly because the current implementation violates the DRY principle.
It would also allow you check against less things per qualifier using if/elif and remove the counting part:
if qualifier_key == qualifier_type_x:
...
elif qualifier_key == qualifier_type_y:
...
else:
print(qualifier_key not supported)
All that being said, it looks like it should all work and won't break or affect other parts of ORION, so it's fine with me to merge in if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be even better to do a lookup of functions or something like that:
semantic_adjustments = {
qualifier_key_1: some_semantic_adjustment_function,
qualifier_key_2: some_other_semantic_adjustment_function,
}
if qualifier_key in semantic_adjustments:
adjusted_qualifier = semantic_adjustments[qualifier_key](qualifier_key, qualifier_value)
Added a script to produce jsonl and neo4j dumps with the qualifiers collapsed into the edge predicate. This should be useful for internal rule mining and heterogenous graph embedding methods where we don't necessarily need to be Biolink compliant.